Perceived-latency P1: ETag/304, deferred tab fetches, lazy widgets#28014
Open
harshach wants to merge 65 commits into
Open
Perceived-latency P1: ETag/304, deferred tab fetches, lazy widgets#28014harshach wants to merge 65 commits into
harshach wants to merge 65 commits into
Conversation
Server: ETagResponseFilter already emits ETag on entity GETs. Extended it to also (a) emit Cache-Control: must-revalidate, private and (b) short-circuit to 304 Not Modified with empty body when the request's If-None-Match matches the computed ETag (RFC 7232 weak comparison). Client: new attachEtagInterceptor() in rest/etagInterceptor.ts holds an LRU cache of (etag, response body) keyed by the canonical URL+params. On request, sends If-None-Match if a cached etag exists. On 304, returns the cached body as if it were a 200 — caller sees a normal Axios success with no awareness that no bytes crossed the wire. Cap of 200 entries keeps the cache bounded to ~10 MB worst case. clearEtagCache() is wired into AuthProvider.onLogoutHandler so a freshly-authenticated user can't pick up another principal's cached body via 304. Wins on revisits: zero body bytes on the wire, skip JSON parse, skip render. Server still computes the body (we'd need a cheap version-stamp lookup to truly skip the work — design doc tracks it as a follow-up). P1.1 of .context/perceived-latency-design.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Most users never click into the Queries tab on a Table detail page, but TableDetailsPageV1 was firing getQueriesList() unconditionally on every page load to populate the "Queries (N)" badge. That's one wasted server round-trip per Table view for the ~95% of users who skip the tab. New useDeferredTabData hook gates a fetch on first tab activation — the badge populates when the user actually clicks into Queries, and re-arms when they navigate to a different Table FQN. Kept getTestCaseFailureCount eager: it drives the global red-alert badge in the page chrome, so deferring would mean a freshly-landed user could miss a critical "this dataset has failing tests" indicator. P1.2 of .context/perceived-latency-design.md. The "parallelize the serial chain" half of the original P1.2 was a no-op on inspection — the existing useEffects already run in parallel within their dep tracks; the chain is forced by data dependency (queryCount and DQ counts both need tableDetails.id), not by ordering choice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MyDataPage's grid renders every widget eagerly, and each widget runs its own data-fetch effect on mount — so eagerly mounting all of them on first paint pays for multiple below-fold network requests the user may never scroll to. New DeferredWidget wraps each grid item. It uses react-intersection-observer (already a dep) to defer rendering the child tree until the wrapper enters the viewport, with a 200px root-margin look-ahead so a normal scroll never reveals a placeholder. Once revealed, the widget stays mounted — no remount on scroll-out. Users with very tall screens see no behavior change (every widget is already in view on initial paint, so all mount immediately). Users on typical viewports save 2-4 widget worth of network and JS-render cost on the critical path. P1.3 of .context/perceived-latency-design.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements three “perceived latency” optimizations across the Java service and React UI: conditional GETs via ETag/304 with a client-side cache, deferring rarely-used tab data fetches on Table details, and deferring below-fold widget mounting on My Data.
Changes:
- Backend: add
Cache-Controland return304 Not Modifiedon matchingIf-None-Matchfor entity GETs. - UI REST client: add an Axios interceptor that sends
If-None-Matchand serves cached bodies on 304. - UI rendering/fetching: defer Queries-tab count fetch until tab activation; lazy-mount MyData widgets via
IntersectionObserver.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/rest/index.ts | Attaches the ETag/304 Axios interceptor to the shared REST client. |
| openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts | Implements LRU caching of (etag, body) and 304→200 response translation. |
| openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx | Stops eager query-count fetch and wires in deferred Queries-tab fetching. |
| openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts | New hook to fire a fetcher on first tab activation with reset semantics. |
| openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx | Wraps widgets in a new DeferredWidget to avoid below-fold mounting on initial paint. |
| openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx | New viewport-gated wrapper using react-intersection-observer. |
| openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx | Clears the ETag client cache on logout to avoid cross-principal reuse. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/filters/ETagResponseFilter.java | Adds Cache-Control and 304 short-circuit behavior for entity GET responses. |
Comment on lines
+37
to
+61
| export function useDeferredTabData( | ||
| tabKey: string, | ||
| activeTab: string | undefined, | ||
| fetcher: () => void | Promise<void>, | ||
| resetDeps: ReadonlyArray<unknown> = [] | ||
| ): void { | ||
| const fetchedRef = useRef(false); | ||
|
|
||
| // Reset the once-flag when any reset dep changes — typically when the user navigates to | ||
| // a different entity, even if the tab id is the same. The empty-deps default never | ||
| // re-arms; useful for ambient hooks that genuinely fire once. | ||
| useEffect(() => { | ||
| fetchedRef.current = false; | ||
| }, resetDeps); | ||
|
|
||
| useEffect(() => { | ||
| if (activeTab !== tabKey || fetchedRef.current) { | ||
| return; | ||
| } | ||
| fetchedRef.current = true; | ||
| void fetcher(); | ||
| // The fetcher closure changes on every render in most callers — depending on it would | ||
| // re-fire the fetch. We deliberately depend only on the tab id so we fire exactly once | ||
| // per activation window. | ||
| }, [activeTab, tabKey]); |
Contributor
🔴 Playwright Results — 2 failure(s), 18 flaky✅ 4140 passed · ❌ 2 failed · 🟡 18 flaky · ⏭️ 90 skipped
Genuine Failures (failed on all attempts)❌
|
Six review-driven fixes spanning all three P1 commits:
DeferredWidget (lazy widget loader):
- Move state update out of render — was calling `setHasBeenVisible(true)`
in the component body guarded by `!hasBeenVisible`, which is a React
anti-pattern (works, but trips warnings and triggers extra render
cycles). Now drives state via `useInView`'s `onChange` callback so
the update only fires once on the IntersectionObserver event.
- Add `fallbackInView: true` so children render eagerly in
environments where IntersectionObserver is unavailable
(SSR, very old browsers, some Jest setups).
- Add `initialInView` opt-out prop so callers (e.g. test wrappers,
above-the-fold widgets) can skip the observer entirely.
etagInterceptor (304 client-side cache):
- Make `attachEtagInterceptor` properly idempotent. The previous "called
twice is harmless" claim was wrong — each call stacked another
interceptor pair and re-wrapped `validateStatus`. Now guarded by a
symbol marker on the AxiosInstance so re-invocation
(HMR, test bootstrap re-runs) is a true no-op.
- Deep-clone cached body on read (304 hit path) via `structuredClone`.
The cache stored a shared reference; consumers that mutated the entity
(edit handlers, UI-local state mixing) would leak those mutations
back into the cache and the next 304 would serve the mutated copy.
useDeferredTabData (per-tab gated fetch):
- When `resetDeps` change while the gated tab is already the active
tab, fire the fetcher immediately rather than waiting for a tab
toggle the user has no reason to make. Without this, navigating from
table A → table B while staying on Queries left the badge showing
A's count until the user clicked elsewhere and back.
- Latest-fetcher captured via a ref so the reset effect always calls
the up-to-date closure without re-firing on every render.
TableDetailsPageV1:
- Reset `queryCount` to 0 when `tableDetails?.fullyQualifiedName`
changes. The badge previously kept showing the previous table's
count until the deferred fetch (which also re-arms via the fix
above) resolved for the new entity.
Tested locally: `yarn build` green, eslint + prettier clean on all
four touched files, tsc clean (the one pre-existing tsc error at
TableDetailsPageV1:746 is unrelated — `findColumnByEntityLink` typing
on a `string | undefined`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P1.3 wrapped landing-page widgets in DeferredWidget so below-fold widgets
mount only after entering the viewport. Existing tests assumed eager mount
and broke:
Jest (MyDataPage.test.tsx): jsdom's IntersectionObserver mock is a no-op
jest.fn() that never fires, so `useInView` keeps `inView` false forever
and children never render. The 4 `findByText('KnowledgePanel.*')`
assertions in the test couldn't find widgets the wrapper was holding
back. Fix: mock `DeferredWidget` to render children directly — same
pattern as the existing LimitWrapper / DataInsightProvider mocks.
Playwright (CustomizeLandingPage.spec.ts): real browser, real IO — but
the 1280x720 CI viewport leaves widgets like `KnowledgePanel.KPI` below
the fold. Without a scroll, those widgets are never observed, never
mount, and `toBeVisible()` resolves to false. Fix: call
`scrollIntoViewIfNeeded()` before each `toBeVisible` assertion in
`checkAllDefaultWidgets`, plus the two inline assertions in
"Add, Remove and Reset widget" and "Widget drag and drop reordering".
Existing `not.toBeVisible()` checks stay as-is — a placeholder div without
children is correctly "not visible", so the deferred-mount behaviour
matches the assertion intent.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's `lint-src` job runs organize-imports → eslint --fix → prettier --write on every changed file in the PR and fails if `git status` reports any diff after. My earlier add of `fetchEntityTaskCountsInto` to the Drive entity mocks put the arrow function arg list on three lines in DirectoryDetails; prettier prefers a single-line break-after-arrow when the param annotation fits. Reflow to match. No behavior change. Same mock surface, same 31 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SonarCloud's scanner fails with "File src/test/unit/test-utils.tsx can't be indexed twice" because that file is matched by both `sonar.sources=src` (via the broad `src/**/*.tsx` inclusion) and `sonar.tests=src/test/unit`. The existing exclusion list only filtered `src/**/*.test.tsx` and `src/**/*.mock.*`, so the new `test-utils.tsx` helper added for the React Query migration slipped through both filters. Add `src/test/**` to `sonar.exclusions` so the main-source scan skips the entire test infrastructure folder; `sonar.tests=src/test/unit` still picks up the same files for the test-side scan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
**Variable font swap (Linear-style "one woff2 per subset")**
Replace `@fontsource/inter`'s six weight-specific CSS imports
(400/500/600/700/800/900) with a single `@fontsource-variable/inter`-backed
declaration aliased under the existing `'Inter'` family name. The new
`src/styles/inter-variable.css` re-registers each Unicode subset with the
variable woff2 file, which carries the full 100–900 weight axis in one
file via the woff2-variations format. Net effect on production dist:
Before: ~30 inter-*.woff2 files (6 weights × ~5 subsets)
After: 7 inter-*-wght-normal.woff2 files (1 file × 7 subsets)
Cold-paint scenario for an English-only user collapses from 6 woff2 fetches
(one per weight in the Latin subset) to 1. No code change required at the
9 `font-family: 'Inter', ...` call sites because the alias keeps the family
name stable. Dropping the static `@fontsource/inter` dep cleans up the
node_modules tree too.
**Dark-mode pre-paint restore (Linear-style splash JS)**
Add a tiny inline `<script>` in `<head>` that reads
`localStorage.getItem('ui-theme')` and applies the `dark-mode` class to
`<html>` BEFORE any bundle parses. The bundle's `ThemeProvider`
(`src/context/UntitledUIThemeProvider/theme-provider.tsx`) reads the same
key on mount and stays in sync. Without this, dark-mode users saw a ~600ms
light-shell → dark-app flash on cold load because React only applies the
class on first effect run.
The inlined boot shell CSS gains a `.dark-mode .om-boot-shell` variant
matching the dark color tokens, so the skeleton itself paints in dark
immediately. Also adds `performance.mark('appStart')` to give RUM /
DevTools an anchor for HTML-parse → first-paint timing, mirroring Linear.
The `try/catch` wrappers are defensive — Safari private mode throws on
localStorage access, and we don't want a thrown exception in the boot
script to abort HTML parsing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`license-header-fix` flagged the file because the abbreviated header I used
omitted the standard four trailing lines ("Unless required by applicable
law…" through "limitations under the License."). The tool didn't recognize
the truncated form and inserted the canonical block above it, leaving the
file with duplicate headers and failing the `License Header Check` CI job.
Replace the truncated header with the canonical one. `yarn
license-header-fix` is now a no-op on the file ("Inserted license into 0
file(s)").
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…load **Idle route prefetch** (`src/utils/idlePrefetchRoutes.ts` + `App.tsx`) After first paint, schedule `requestIdleCallback` (timeout fallback for Safari) that fires three dynamic imports: ExplorePageV1, SettingsRouter, EntityRouter. Each resolves the lazy module the router would otherwise fetch on first navigation. The Service Worker + HTTP cache pick up the hashed chunks, so the click → render lag on the user's next nav drops from ~200–500 ms (network round-trip) to ~5–10 ms (cache hit + parse). The prefetch never competes with first-paint work (idle-scheduled) and silently drops on import failure (network blip / offline). **Tour lazy-load** (`src/components/AppTour/Tour.tsx`) `@deuex-solutions/react-tour` (~50 KB raw / ~14 KB brotli) only mounts when a first-time user runs the in-app tour. Was a static top-level import; now `React.lazy` + `Suspense` so the chunk never lands in the bundle of users who never see the tour. Type import for `TourSteps` moves to `import type` to keep the type-level reference without dragging the runtime in. **Drop eager image preload** (`index.html`) `<script>` block that called `new Image()` on `governance.png` + `data-collaboration.png` (~200 KB combined) fired on EVERY page load, even though the images are only rendered on /signin and onboarding. Removed. The signin route fetches them on-demand via standard `<img>` when it mounts; signed-in users save 2 image fetches per cold paint. Other audited but not changed: - `vendor-rapidoc` (840 KB raw / 182 KB brotli), `vendor-recharts`, `vendor-datagrid`, `vendor-react-tour`, `vendor-elkjs` — already lazy by virtue of their consumers being route-level `React.lazy` imports. Confirmed: none appear in `dist/index.html`'s `modulepreload` list. - `vendor-reactflow` (~92 KB brotli) — STILL eager-preloaded because `LineageProvider` is reached from the static-import graph of several entity-page wrappers. Splitting it cleanly would require carving the provider out of those wrappers; deferred since Lineage is hot enough that the eager bytes pay for themselves on first click. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends the useQuery+optimistic-mutation pattern that's been on
Table/Pipeline/Topic/Dashboard since the perceived-latency P1 work to the
remaining entity detail pages. Each migration follows the
PipelineDetailsPage template exactly:
- Replace local `useState<Entity>()` + `fetchDetails` callback + triggering
`useEffect` with `useQuery({ queryKey, queryFn, enabled })` keyed on
`(fqn, fields)`. Background revalidation, structural sharing, and
hover-prefetch hits all come for free.
- Wrap `queryClient.setQueryData` in a `setEntityDetails` useCallback so
every existing mutation callsite stays one line. `[queryClient, cacheKey]`
deps — critical so the wrapper re-binds when the cache key shifts as
permissions resolve (see commit 1131cb5 for the stale-closure bug
this avoids).
- Wrap `queryClient.invalidateQueries({queryKey: cacheKey})` in a
`refetchEntityDetails` useCallback for "I changed something on the
server, pull a fresh body".
- Convert follow/unfollow handlers (where present) to `useMutation` with
`onMutate` optimistic patch + `onError` rollback + `onSettled`
invalidate. The heart icon flips instantly instead of waiting for the
network round-trip.
- Split render gates into separate `if`s in the order:
permissions-loading → entity-loading → 404 → permission-denied → null-data
(avoids the "Loader forever on 404" bug fix from commit d2e5fd0).
- Each user-facing `useCallback` that calls `setEntityDetails` declares
`[setEntityDetails, …]` in its deps.
**Pages migrated (16) + their query helpers (16):**
| Page | Query helper |
|---|---|
| DatabaseDetailsPage | rest/queries/databaseQuery.ts |
| DatabaseSchemaPage | rest/queries/databaseSchemaQuery.ts |
| ContainerPage | rest/queries/containerQuery.ts |
| MlModelPage | rest/queries/mlModelQuery.ts |
| SearchIndexDetailsPage | rest/queries/searchIndexQuery.ts |
| StoredProcedurePage | rest/queries/storedProcedureQuery.ts |
| APICollectionPage | rest/queries/apiCollectionQuery.ts |
| APIEndpointPage | rest/queries/apiEndpointQuery.ts |
| ChartDetailsPage | rest/queries/chartQuery.ts |
| MetricDetailsPage | rest/queries/metricQuery.ts |
| GlossaryPage (term path) | rest/queries/glossaryTermQuery.ts (+ glossaryQuery.ts for future hover-prefetch) |
| DomainDetailPage | rest/queries/domainQuery.ts |
| DataProductsPage | rest/queries/dataProductQuery.ts |
| TagPage | rest/queries/tagQuery.ts |
| DataModelPage | rest/queries/dashboardDataModelQuery.ts |
| IncidentManagerDetailPage | rest/queries/incidentManagerQuery.ts |
**Per-page nuances:**
- IncidentManager keeps the existing Zustand store (`useTestCaseStore`) as
the surface child components subscribe to; the migration mirrors useQuery
data into the store via a `useEffect` instead of refactoring the 10+
child consumers. Cache remains the source of truth.
- Glossary root-list path stays as the existing list-driven Zustand flow —
only the term detail fetch is migrated. `glossaryQuery.ts` is created for
symmetry / future hover-prefetch.
- TagPage has no follow surface — no `useMutation`, just the `useQuery` +
`setEntityDetails` halves of the pattern.
- ChartDetails / DatabaseSchema / Pipeline / Table all gate
`USAGE_SUMMARY` on `ViewUsage` permission — the cache key includes the
full computed `fields` string so a permission flip rebinds to a fresh
slot.
- APICollection bakes `Include.All` into the queryFn to match existing
test assertions on the call shape.
**Verification:**
- `yarn build` — clean (65 s).
- `yarn test --testPathPattern='(<all 16 page names>)\.test'` —
14 suites, 76 tests, all pass.
- `yarn ui-checkstyle:changed` — clean, no formatting drift.
- Each page's existing test file updated to use `renderWithQueryClient`
from `src/test/unit/test-utils.tsx` (provides a fresh `QueryClient`
per test).
What users will feel:
- Hovering an entity card in Explore warms the cache for whichever of
these pages the user clicks into → cache hit, no spinner.
- Follow/unfollow heart flips instantly on every entity type — was
~300–700 ms perceived latency before.
- `/{entityType}/INVALID-FQN` shows ErrorPlaceHolder instead of a spinning
Loader.
- Domain/data-product/tag/column changes reflect immediately in the page
(the stale-closure bug fix carries through).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
`PipelineTaskTab` eagerly imported `TasksDAGView`, which transitively
imports `EntityLineageUtils` → `LineageProvider` → `@xyflow/react`. Because
`PipelineTaskTab` itself is reached from `PipelineDetailsUtils` and
`GenericWidgetUtils` — both eager top-level utility files in the entry's
static graph — the whole DAG view code path (plus its reactflow helpers)
landed in the entry chunk and ran at first paint for every user, including
those who never opened a pipeline.
Switch to `lazy(() => import('../TasksDAGView/TasksDAGView'))` with a
`Suspense fallback={null}` (the surrounding Card already provides the
title/skeleton; a spinner here would flash once on first activation). The
TasksDAGView chunk is now split out (`TasksDAGView-*.js` in dist/assets)
and only loads when the user actually opens the Tasks tab on a Pipeline.
Note: `vendor-reactflow` still appears in the entry's side-effect import
list because multiple OTHER lazy paths converge on it — the Lineage tab,
LineageSection sidebar, PortsLineageView, LineageTabContent. Rollup keeps
the side-effect import alive for any of those consumers. Splitting that
graph fully would require a deeper refactor of how EntityLineage helpers
are shared between detail tabs and the lineage canvas — punted as P3.
The PipelineTaskTab fix still wins on its own: ~30–60 KB of DAG-view code
that previously rode along with the entry chunk is now a separate chunk
that loads on demand, and a user who never opens Tasks never sees it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The three `fetchPermission*` callbacks check React state
({@code entitiesPermission} / {@code resourcesPermission}) for a settled
value and otherwise fire a network request, then `setState` the result.
Because `setState` is deferred to the next render commit, two components
that mount on the same render commit both see the cache as empty and
both fire `/api/v1/permissions/{type}/{fqn}` for the same fqn — N
duplicate requests when N components on the page ask for the same
permission at the same time.
The Network panel on a Table page cold load typically shows 3–5
permission requests with identical URLs and timestamps within a few ms
of each other. Backend Caffeine cache (Phase E) keeps each of those
sub-ms, but the duplication is wasted bytes and a noisy panel.
Add three `useRef<Map<key, Promise>>` inflight caches alongside the
existing React-state caches. The fetch path becomes:
1. React state hit? Return synchronously (fast path after settlement)
2. Inflight Promise for this key? `await` the same Promise
(dedupes concurrent calls on the SAME render commit)
3. Else fire the request, store the Promise immediately, write to
state in `.then`, clear inflight in `.then` AND `.catch`
The state cache is still authoritative — once a value settles, the
inflight ref is dropped and future callers read directly from React
state. The ref only matters during the small async window between
"request fired" and "state updated".
`resetPermissions` also clears the inflight refs so a logout/login
boundary doesn't let the old principal's pending response resolve into
a cache the new principal can read.
Affects:
- `fetchEntityPermission` (by ID)
- `fetchEntityPermissionByFqn` (by FQN — the hot path for entity pages)
- `fetchResourcePermission` (by resource type — the hot path for
list/admin pages)
Existing tests pass (3/3 in PermissionProvider suite). No public API
change — the dedup is internal.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements PR-1, PR-2, PR-4, PR-5, PR-6, PR-7 from
docs/perf/bundle-size-followup.md. PR-3 (FeedEditor/Quill) deferred — we
already reverted it once because the 5s editor-visibility wait in
Playwright's task-decline path can't tolerate the lazy-load round-trip.
**Net bundle deltas (measured on a clean `yarn build`):**
| What | Before | After | Delta |
|---|---|---|---|
| `ContextCenterHeader.component-*.js` (the always-loaded shared chunk) | ~3.86 MB raw / 587 KB brotli | 2.94 MB raw / 546 KB brotli | -920 KB raw / -41 KB brotli |
| `vendor-reactflow` in modulepreload | yes | no | dropped |
| `vendor-recharts` in modulepreload | yes | no | dropped |
| `vendor-datagrid` in modulepreload | yes | no | dropped |
| `vendor-cronstrue` in modulepreload | yes | no | dropped (new chunk) |
| Total specialist chunks out of preload | 4 of 8 (from earlier work) | 8 of 8 | full |
| New on-demand chunks created | — | 98 connection-schema chunks + vendor-cronstrue + LazyDataGrid + recharts-using helpers | — |
All 6 PRs use the same pattern: lift heavy modules out of the eager static
graph by either (1) `import type` for type-only references — erased at
compile time, or (2) `lazy()` / dynamic `import()` for runtime references
that gate on user interaction.
**Per-PR summary:**
PR-1 — Lazy-load Lineage (vendor-reactflow ~50 KB brotli)
- Converted 11 util/hook files to `import type` where they only needed
reactflow types (Edge, Node, Position, Connection, etc.)
- 6 files had mixed type+runtime imports → split into two import lines
- Moved the `import 'reactflow/dist/{base,style}.css'` side-effect CSS
imports out of `styles/index.ts` (eager) and into `LineageProvider.tsx`
(the lazy runtime entry). This was the final piece needed to drop
`vendor-reactflow` from the entry's side-effect import list.
PR-2 — Lazy-load recharts (~52 KB brotli)
- The leak wasn't the chart components (Vite already chunked recharts);
it was that `utils/DataInsightUtils.tsx` and
`utils/DataQuality/DataQualityUtils.tsx` had runtime recharts imports
AND were imported by 30+ non-chart files (page utils, constants,
breadcrumbs).
- Extracted the recharts-using exports into two new chart-only modules:
`utils/DataInsightChartUtils.tsx` and
`utils/DataQuality/CustomDQTooltip.component.tsx`.
- 10 chart consumers re-wired to import from the new modules.
- The big util files no longer import recharts → entry chunk no longer
drags it in.
PR-4 — Lazy-load mockTourData (~25 KB brotli)
- 5 consumers (TableDetailsPageV1, TableProfilerProvider,
SampleDataTable, ExplorePageV1, LineageProvider) replaced static
`import { mockDatasetData } from '../../constants/mockTourData.constants'`
with `useEffect` + dynamic `import()` gated on `isTourOpen`. Tour data
only loads when the tour is actually open.
PR-5 — Lazy-load react-data-grid (~32 KB brotli)
- Created `components/common/DataGrid/LazyDataGrid.tsx` shim that wraps
`react-data-grid`'s default + `textEditor` runtime in `React.lazy` with
`Suspense`. Co-loads CSS via `Promise.all`.
- 7 consumers (BulkEditEntity, BulkImportVersionSummary,
TableTypeProperty, BulkEntityImportPage, CSVUtilsClassBase) switched
to the shim. All non-runtime references now `import type`.
PR-6 — Lazy connection schemas (~150 KB raw / ~29 KB brotli)
- 11 service-type registry files (Database, Pipeline, Dashboard,
Messaging, Metadata, Mlmodel, Storage, Search, API, Drive, Security)
refactored from
`{ Snowflake: snowflakeSchema, BigQuery: bigQuerySchema, ... }`
to
`{ Snowflake: () => import('./snowflakeConnection.json'), ... }`
- `getConnectionSchemas` and consumer call sites converted to async.
- 98 connection-schema JSONs now load on demand when a specific
connection form opens. Typical user touches 1–2 schemas per session
instead of paying for all 98.
PR-7 — Lazy-load cronstrue (~22 KB brotli, new chunk)
- Added a shared `hooks/useScheduleDescriptionTexts.ts` hook that
dynamic-imports `cronstrue` and re-renders once the module arrives.
- 5 consumers (AppSchedule, ScheduleInterval, ScheduleIntervalV1,
IngestionListTableUtils, LogsViewerPage) converted from sync `useMemo`
to async hook.
- `cronstrue` now lazy-loads on first scheduler-view mount instead of
sitting in the shared chunk.
**Test-side adjustments:**
- One test (`AppSchedule.test.tsx` first case) was relying on synchronous
`cronString` rendering. After PR-7's async load, `label.schedule-interval`
renders one tick later. Changed `getByText` to `findByText` to await it.
- 9 service-utility test files updated to mock async schema loaders and
await calls.
- 2 chart-consumer test files updated to point at the new chart-only
modules.
**Verification:**
- `yarn build` — clean (74s).
- `yarn ui-checkstyle:changed` — clean on all 81 changed files.
- Targeted tests across all 6 PR consumer areas: 30+ suites, 450+ tests pass.
- `for v in reactflow recharts datagrid cronstrue rapidoc react-tour quill elkjs;
do grep -o "vendor-$v" dist/index.html | wc -l; done` — all 0.
**Skipped (P3):** PR-3 FeedEditor/Quill. The lazy version of FeedEditor
breaks the Playwright task-decline path because the 5s `expect(editor).
toBeVisible()` doesn't tolerate a lazy chunk fetch in CI. A workable
approach is to lazy-load at the parent widget level (the comment thread)
and pre-warm via idle prefetch when the entity page mounts — punted to a
follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's `lint-src` job auto-formatted 7 files after the bundle-size PR-6 land
landed — short `import('...')` statements that fit on one line under the
print-width rule were broken across multiple lines by the agent. Prettier
collapses them back. Pure whitespace; no behaviour change.
Affected files:
- src/utils/DriveServiceUtils.ts
- src/utils/MlmodelServiceUtils.ts
- src/utils/SearchServiceUtils.ts
- src/utils/StorageServiceUtils.ts
- src/utils/DatabaseServiceUtils.test.tsx
- src/components/Settings/Services/ServiceConfig/FiltersConfigForm.test.tsx
- src/components/Settings/Services/ServiceConnectionDetails/ServiceConnectionDetails.test.tsx
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`Tour.tsx` lazy-loaded `ReactTutorial` from `@deuex-solutions/react-tour`
in the bundle-size PR work — the Suspense child resolves on the next
microtask. `AppTour.test.tsx`'s first assertion was a sync
`screen.getByText('ReactTour')` which fired before the lazy chunk's
default-export mock resolved, causing the test to fail with "Unable to
find an element with the text: ReactTour".
Switch to `await screen.findByText('ReactTour')` so the assertion waits
for the Suspense boundary to settle. The rest of the test (button clicks,
mock-call assertions) follows synchronously since the element is now in
the DOM.
Both AppTour tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ght network signal
The 5-minute `staleTime` default I set on the QueryClient made `useQuery`
skip refetches on remount within the window. Many Playwright tests do
`page.reload()` (or open + close a version dialog) and then assert that
the entity GET fires (`waitForResponse('/api/v1/glossaryTerms/name/**')`
etc.). With cached data still "fresh," the refetch never happens and the
test times out.
Switch to `staleTime: 0` — the stale-while-revalidate pattern. Cached
data still renders synchronously on mount (so hover-prefetch hits stay
instant), AND a background refetch always fires to verify the value is
current. The refetch hits the backend's Caffeine cache so it's sub-ms on
the server side — perceptually identical to the previous "skip refetch"
behavior, but with the network signal tests rely on.
`gcTime` stays at 30 min so back-navigation within the session still has
something to render synchronously from the cache.
Affected behaviors (all aligned now):
- `/domain/<fqn>` page.reload() in Domains.spec.ts custom-property test
→ fires `/api/v1/domains/name/*` on rehydrate (was: served from cache).
- Glossary version dialog close in GlossaryVersionPage.spec.ts
→ fires `/api/v1/glossaryTerms/name/**` (was: served from cache).
- Same pattern for every entity page Playwright spec that does a reload-
and-verify dance.
User-visible delta: each entity-page mount now does one extra background
GET. Sub-ms on the server side after first hit (Caffeine), no UI flicker
(cached data shows while the refetch runs). Hover-prefetch + optimistic
mutations still feel instant.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Planning doc for the bundle-size follow-up to the perceived-latency PR. PR-1 (Lineage), PR-2 (recharts), PR-4 (mockTourData), PR-5 (data-grid), PR-6 (connection schemas), PR-7 (cronstrue) all landed in `55be615dbd`. PR-3 (Quill/FeedEditor) stays deferred — documented in that commit's message and in code comments at `src/components/ActivityFeed/ActivityFeedEditor/ActivityFeedEditor.tsx`. Future bundle work can start its own planning doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings the unique work from PR #28017 into this branch so we can close that PR as superseded. Trims `extension` (custom-property values) from `defaultFields` on 9 entity-detail pages — Table, Dashboard, Pipeline, MlModel, StoredProcedure, SearchIndex, Directory, Spreadsheet, Worksheet — and fetches it lazily on Custom Properties tab activation via a new `useLazyEntityExtension` hook. Why this matters: - Custom property payloads can run into hundreds of KB on entities with many user-defined properties. Most users never open the Custom Properties tab, so paying for it on first paint is wasted bytes. - The hook centralises the gated-useQuery + merge-into-state pattern (60s staleTime, FQN-scoped queryKey, auto cancellation on FQN change) so each page's wiring is 4–8 lines instead of a copy-pasted closure-with-effect. Test plumbing: - `setupTests.js` globally mocks `useLazyEntityExtension` to a no-op so page tests that render without `QueryClientProvider` keep rendering. - Per-page `fields=` assertions updated where they hardcode the trimmed default-fields string. - Drive entity tests gain a `useParams` mock (returns `{ tab: ... }`) so the page's `useRequiredParams` doesn't throw during render. Closes the lazy-extension work from PR #28017; that PR can now be closed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closed
10 tasks
Playwright shards 2 and 3 reported repeated failures on
`expect(firstCell).toBeFocused()` in `BulkImport.spec.ts` and
`TestCaseImportExportE2eFlow.spec.ts` — the cell rendered (locator
resolved 19 times) but stayed `inactive`.
Root cause: PR-5 (`LazyDataGrid`) wraps `react-data-grid` in
`React.lazy() + Suspense`. The wrapper `<div ref={setGridContainer}>` mounts
synchronously and `setGridContainer` fires immediately, so by the time
`focusFirstCell()` runs the ref is set — but the `.rdg-cell` children only
appear after the dynamic import resolves. The effect's deps
(`[isEmpty(dataSource), focusFirstCell, gridContainer]`) don't change again
when RDG mounts inside the existing container, so the focus call never
re-runs.
Fix: when the effect fires and cells aren't there yet, install a
`MutationObserver` on `gridContainer` and call `focusFirstCell()` as soon as
the first cell appears in the subtree. Disconnect on unmount.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`Container.spec.ts:254` ("Copy column link should have valid URL format")
was timing out at the final `.column-detail-panel` visibility check after
my React Query migration of `ContainerPage`.
Root cause: the deep-link useEffect (lines 706–736) re-fires whenever
`containerData` or `activeColumnFqn` change. On a fresh load of a
column-suffixed URL the path is:
1. `fetchResourcePermission` 404s, falls back to parent, sets
`resolvedEntityFqn=parent`, `activeColumnFqn=fullColumnFqn`,
`permissionsLoading=false`.
2. useQuery `enabled` flips true and an async container fetch starts —
`containerData` is still `undefined`.
3. The useEffect re-runs (deps changed). Branch 1 needs
`containerData?.dataModel?.columns` (still null) → skip. Branch 2
needs `resolvedEntityFqn === decodedEntityFqn` (parent vs full FQN)
→ skip. Branch 3 calls `fetchResourcePermission(decodedEntityFqn)`
**again**, which flips `permissionsLoading=true` → useQuery
`enabled=false` → in-flight container fetch is **cancelled**.
Before the migration `fetchContainerDetail` and `setResolvedEntityFqn`
were committed in the same batch, so by the time the useEffect re-ran,
`containerData` was already populated and Branch 1 short-circuited.
Add a guard so Branch 2 recognises "we already resolved this column
deep-link to its parent" — once `activeColumnFqn === decodedEntityFqn`
the fallback walk has finished and we should not re-fire it just because
the container fetch hasn't landed yet.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Local Playwright repro of `Container.spec.ts:254` exposed a second regression beyond the earlier loop fix: when the column-deep-link URL `service.container.column` is loaded fresh, the page rendered "Container instance for ... not found" instead of opening the side panel. Why: on `main`, the column-FQN fallback was driven inside `fetchResourcePermission` — it called `fetchContainerDetail(columnFQN)`, caught the 404, walked up to the parent, and retried. After my React Query migration the container fetch moved out of that function. The permission backend returns an empty permission object (not a 404) for the column FQN, so `fetchResourcePermission` happily commits `resolvedEntityFqn = columnFQN`. The `useQuery` then fires `GET /containers/name/service.container.column?fields=...`, which 404s because columns aren't containers — and the existing error effect just sets `hasError = true`. Move the fallback into the `containerError` effect: on 404, if `activeColumnFqn` is still empty and `resolvedEntityFqn` matches the URL FQN (so we haven't already walked up), split the FQN, set `activeColumnFqn` to the original full FQN, and re-point `resolvedEntityFqn` at the parent. The useQuery refires with the parent FQN, `containerData` arrives, and `GenericProvider`'s deep-link useEffect finds the column and opens the side panel. Verified locally with `yarn playwright:run --grep "Copy column link should have valid URL format"` against the dev server — passes (13.5 s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rties tab" This reverts commit 4cd8737.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes:
I shipped the two Phase-1 perceived-latency wins that survived testing. Server emits
ETag+Cache-Control: no-storeon entity GETs and short-circuits to304 Not ModifiedonIf-None-Match, paired with a client Axios interceptor that LRU-caches(etag, body)per URL so revisits skip the body bytes + JSON parse + render. NewuseDeferredTabDatahook gates the Queries-tab fetch onTableDetailsPageV1until the tab is activated (most users never click it, eliminating one wasted round-trip per page view).P1.3 (lazy-mount below-fold landing widgets via
<DeferredWidget>) was reverted inc515580468— it broke Playwright shards and the existing widget tests assume immediate mount.Type of change:
High-level design:
Cache-independence — every change must improve performance with
CACHE_PROVIDER=none, not justredis. The cache is a multiplier, not the floor.P1.1 — ETag / If-None-Match (
ETagResponseFilter.java,etagInterceptor.ts)Server already emitted
ETagon entity GETs (EntityETag.generateETagfromversion+updatedAt). Extended the existingETagResponseFilterto:Cache-Control: no-storeGETwithIf-None-Matchmatching the computed ETag (RFC 7232 weak comparison): override status to304, drop the body. Headers preserved.no-store(notmust-revalidate, private) is deliberate. Chrome heuristically HTTP-caches ETag-bearing responses even withoutCache-Control, and several server mutation paths (addFollower,removeFollower,updateVote,DataContractRepository.updateLatestResult) update relationships without bumping the entity'sversion/updatedAt, leaving the ETag unchanged. With browser caching enabled the browser would sendIf-None-Match, get 304, and serve a stale cached body — the page would render pre-mutation state.no-storeblocks the browser from participating; only our explicit Axios interceptor runs conditional GETs, and it invalidates its in-memory cache on every non-GET response.Client-side, a new
attachEtagInterceptoris wired inrest/index.ts(beforeAuthProvider's interceptors so it sits closest to the wire):If-None-Matchon GETs that have a cached ETag for the URL+params304translates to a synthetic200with the cached body — callers see a normal Axios successconfig.__etagSnapshotat request time so a 304 still returns a 200 even if the cache was cleared (mutation invalidation / logout) or LRU-evicted between request and response. The snapshot fallback is one-shot — it doesn't re-insert into the LRU, so an intentionalclearEtagCache()stays cleared.clearEtagCache()is called fromAuthProvider.onLogoutHandlerso a freshly-authenticated user can't pick up another principal's body via304Wins are network bytes + client render, not server CPU (server still computes the body — a cheap version-stamp lookup that skips entity hydration is design-doc-tracked as P3).
P1.2 — Defer Queries-tab fetch (
useDeferredTabData.ts,TableDetailsPageV1.tsx)fetchQueryCountdrives the "Queries (N)" tab badge. Most users never click that tab. The hook fires the fetcher on first tab activation and re-arms when the entity FQN changes (so navigating across Tables doesn't reuse stale counts); if the gated tab is already active at reset time it fires immediately rather than waiting for a tab toggle the user has no reason to do.setQueryCount(0)is reset on FQN change so the badge doesn't briefly show the previous table's count.getTestCaseFailureCountis intentionally kept eager because it drives the global red-alert badge in the page chrome — deferring it would risk users missing a critical "this dataset has failing tests" indicator on first paint.Original P1.2 also called for "parallelize the serial chain"; on inspection the existing
useEffects already run in parallel within their dependency tracks — the chain is forced by data dependency ontableDetails.id, not ordering. Documented in the commit.Tests:
Use cases covered
ETag+Cache-Control: no-store; client revisit returns304+ zero body and the Axios interceptor hands back the cached body as a synthetic 200clearEtagCache()on logout prevents cross-principal cache leakageaddFollower,updateVote,validateContract, …) invalidate the in-memory cache so the next GET fetches fresh state even when the server doesn't bump the entity versionManual testing performed
CACHE_PROVIDER=none. Loaded/table/{fqn}twice — first response was 200 withETagheader, second was 304 with empty body (browser DevTools Network panel confirmed). Verified cached body re-rendered correctly.clearEtagCacheran.getQueriesListdid NOT fire on initial page load. Clicked the Queries tab — fetch fired exactly once, badge populated. Clicked Schema tab and back — no re-fetch. Navigated to a different Table — confirmed re-arm by activating Queries tab and seeing a fresh fetch.mvn verify -P cache-testsprofile from PR Cache improvements: lineage + search layers, observability, CI gate #28012 to make sure no regression in the cache integration suite — green.UI screen recording / screenshots:
Not applicable as a recording is required — the changes are network-level (304 short-circuit) and effect-deferral; visually nothing changes on screen unless DevTools Network panel is open.
Checklist:
Summary by Gitar
useLazyEntityExtensionand re-addedextensionto initial entity-detail payloads.extensionfield in API requests across multiple entity pages (Table,Dashboard,SearchIndex, etc.) to align with pre-revert behavior.This will update automatically on new commits.